Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GDScript: Improve error messages for invalid indexing #82639

Merged

Conversation

golfinq
Copy link
Contributor

@golfinq golfinq commented Oct 1, 2023

Take over of #66948

Changes from the pull request are the addition of enums which specifies why the variant getter/setter failed, and changes the message to a more useful one on failure of getting/setting an index

@golfinq golfinq requested review from a team as code owners October 1, 2023 20:24
@golfinq golfinq force-pushed the gdscript-improve-indexing-error branch from ed922d5 to e9a7613 Compare October 1, 2023 20:31
@Calinou Calinou added this to the 4.2 milestone Oct 1, 2023
@golfinq golfinq force-pushed the gdscript-improve-indexing-error branch from e9a7613 to c6becd5 Compare October 1, 2023 21:11
@golfinq
Copy link
Contributor Author

golfinq commented Oct 2, 2023

Should I edit the failing tests to accommodate the new error messages?

@golfinq
Copy link
Contributor Author

golfinq commented Oct 2, 2023

logged: Make sure *.out files have expected results.
All GDScript tests should pass.

Yep

These errors are very common when using an invalid property name
or calling on an object of the wrong type, and the previous message
was a bit cryptic for users.

Co-authored-by: Rémi Verschelde <[email protected]>
Co-authored-by: golfinq <[email protected]>
@golfinq golfinq force-pushed the gdscript-improve-indexing-error branch from c6becd5 to 5efbed5 Compare October 2, 2023 18:23
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-approved during the GDScript team meeting. Members of the team have been asked to check the PR and the errors formulation in order to make sure that they are correct, but for the most part, this seems like a great improvement.

This raises the question about Godot supporting global error codes, both in GDScript and in the Godot core itself.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These things are always hard to get your head around the wordings but they sound right and as an improvement to me 🎉

@akien-mga akien-mga merged commit 2bffa3c into godotengine:master Jan 4, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants